Add library for humility dump#698
Conversation
labbott
force-pushed
the
humility-dump-lib
branch
2 times, most recently
from
9fb35b1 to
863bb7d
Compare
| .map_err(DumpError::UdpAgent)?, | ||
| )) | ||
| } else { | ||
| // XXX |
There was a problem hiding this comment.
I think this was a reminder that I grabbed a timeout instead of having it passed in. I'm also looking at the code and timeout doesn't get used anywhere except humility-net-core which is the exact opposite case here! I might take a pass to fix this up.
There was a problem hiding this comment.
I'd still like to fix this but I don't know if I want to depend on this so I cleaned it up with a comment for now.
| pub fn take_system_dump_via_agent( | ||
| hubris: &HubrisArchive, | ||
| core: &mut dyn Core, | ||
| dumpfile: Option<&PathBuf>, |
There was a problem hiding this comment.
I don't love having the naming logic in the library (I would be happier if this took a std::io::Write handle), but that's annoying to fix right now since it's actually in HubrisArchive::dump.
There was a problem hiding this comment.
I took a pass at pulling this out. It wasn't too bad.
| let headers = | ||
| agent.read_dump_headers(false).map_err(DumpError::DumpHeaders)?; | ||
|
|
||
| if headers.is_empty() || headers[0].0.dumper == humpty::DUMPER_NONE { |
There was a problem hiding this comment.
Nit: because we pass raw: false to read_dump_headers, is it even possible to get DUMPER_NONE?
There was a problem hiding this comment.
("This is how it worked before and I don't want to deal with it in this PR" is a valid response here)
There was a problem hiding this comment.
Yeah I was copy/pasting the existing logic. I think you are correct but I'm lightly wary to remove it without more testing. I might just add another comment.
This function has grown complicated and hard to follow. Extract the following to separate functions - `subargs.initialize_dump_agent` - `subargs.simulate_dumper`/`subargs.emulate_dumper`/ `subargs.simulate_task_dump` - `subargs.extract`/`subargs.force_read` There's a little bit of redundancy but it makes the logic much easier to follow.
One of the most common uses for humility is to take an SP dump on a system so it can be analyzed later. Having this as a library function makes it easier to do it more places.
This reverts commit 9552bf5.
labbott
force-pushed
the
humility-dump-lib
branch
from
863bb7d to
38b5774
Compare
| let mut file = std::fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .open(stock)?; |
There was a problem hiding this comment.
Nit: this no longer prints dumping to {filename}; I'm ambivalent on whether that matters
| Ok(()) | ||
| } | ||
|
|
||
| /// Take and collect a dump via the agent. If a file is not |
There was a problem hiding this comment.
Docstring is now outdated, since we pass in a std::io::Write
| likely due to space exhaustion; \ | ||
| dump will be extracted but may be incomplete!" |
There was a problem hiding this comment.
Nit: indentation
| likely due to space exhaustion; \ | |
| dump will be extracted but may be incomplete!" | |
| likely due to space exhaustion; \ | |
| dump will be extracted but may be incomplete!" |
| } | ||
|
|
||
| /// List all available dumps | ||
| pub fn list_dumps_via_agent( |
There was a problem hiding this comment.
Nit: rename to list_dumps, because there's no DumpAgent in the signature?
|
Looks good, approved with a few nits |
2 participants
This is broken up into two commits
humility dumpto make some of the logic easier to followBecause of the way
humility dumpcurrently works it's tricky to have all the library functions replace the same functions in the binary. Almost tempted to say we should start from scratch withhumility dump-ngbut that's probably a bad idea.